Check __cpp_lib_unreachable before using std::unreachable in xxhash.h - #15655

Findings

🟡 MEDIUM

M1. #include <utility> guard still uses language version, not feature-test macro — util/xxhash.h:18

• Issue: The RocksDB customization block at lines 18-21 that #include <utility> still guards on __cplusplus > 202002L. With the proposed change at line 2075, the XXH_UNREACHABLE macro now checks __cpp_lib_unreachable >= 202202L, but that macro is defined by <utility>. If a compiler is in C++20 mode but the standard library defines __cpp_lib_unreachable as a backport/extension (unlikely but possible), the <utility> include would be skipped, __cpp_lib_unreachable would not be defined, and the code would silently fall through to __builtin_unreachable() — which is safe but inconsistent with the intent.
• Root cause: The include guard and the usage guard were updated independently. They should use the same condition, or the include should be unconditional for C++ (since <utility> is always available in C++).
• Suggested fix: Either change line 18 to also check __cpp_lib_unreachable (chicken-and-egg: you need to include <utility> to get the macro), or simply include <utility> unconditionally in C++ mode so the feature-test macro is available. However, this is a vendored file and the current approach still works correctly: in practice, any compiler with __cpp_lib_unreachable will also report __cplusplus > 202002L, so the include fires. The fallback to __builtin_unreachable() is always safe. This is low practical risk.
• Severity rationale: Medium because the two guards are logically inconsistent, even though in practice they won't diverge on any known compiler. A future toolchain could theoretically expose the gap.

🟢 LOW / NIT

L1. Comment accuracy — util/xxhash.h:18-19

• Issue: After the change, the comment at line 19 says "C++23 standard libraries may have std::unreachable()" which is accurate, but the guard at line 18 still says __cplusplus > 202002L (C++23 language version). The comment and guard are consistent but the semantic has shifted: the include is now needed to detect the feature (via __cpp_lib_unreachable), not just to use it. A more precise comment would be: /* Check for std::unreachable() via __cpp_lib_unreachable */.

L2. Consider upstream sync — util/xxhash.h

• Issue: This is a vendored file with RocksDB-specific customizations. The fix aligns with upstream xxHash's recommended approach for std::unreachable() detection. When upstream xxHash next updates this logic, RocksDB's vendored copy should be synced to pick up the fix natively, at which point the RocksDB customization block (lines 15-22) may become unnecessary.

Cross-Component Analysis

Positive Observations

• The fix is minimal and surgical — only the detection condition changes, preserving the fallback cascade.
• The change matches the C++ standard's recommended way to detect library features via feature-test macros.
• No behavioral change for any correctly-functioning build; only fixes broken builds.
• The PR description clearly explains the motivation and scope.

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase